I created a document to illustrate the storage concept presented in this PR and discuss the new equations suggested to be added to the model. I put it under a "temporary" folder in the doc folder. Please feel free to put in the right place.

Thanks @behnam2015 for the PR, I only have some comments and suggestions on the GAMS side.

period_parameter_assignment.gms
I tried the model with different naming for the 'time' elements (i.e non alphabetic order and with different numbers of letters) and there is a problem on the Python side, list(set(seasons_dict.keys())) is re-ordering the set alphabetically, this should not happen, so that one can have seasons like winter, summer and so on.
Fixing that (but I did it hard coding), then the new ordering is correct on the GAMS side.
However, I would suggest making time_seq a set and defining some basic sets that can be helpful in the future, such as first_time(time) last_time(time).

This is an example (not tested), but if we agree I can push changes to your branch:

Sets
first_time(time)
last_time(time)
;

first_time(time)$( ORD(time)$time_seq(time) = 1) = yes ;
last_time(time)$( ORD(time)$time_seq(time) = CARD(time_seq(time) ) = yes ;

* mapping of first and last time steps in a period AV
map_time_first_last(year_all,year_all2,time,time2)$( ( ORD(year_all) = ORD(year_all2)) AND
    first_time(time) AND  last_time(time) ) = yes;

Concerning STORAGE_CHANGE equation, if the level_storage is always different from the normal commodity levels (e.g charger output = discharger input but different than charger input) then we don't need all the charger and discharger mappings and we can write a single line:

STORAGE_CHANGE(node,storage_tec,level,year,time)$( SUM( (mode,tec,commodity), map_tec_storage_level(node,tec,storage_tec,level,year,time) AND
    (map_tec_charge(node,tec,mode,commodity,level,year,time) OR map_tec_discharge(node,tec,mode,commodity,level,year,time) ) ) )..
* change in the content of storage in the examined timestep
    STORAGE_CHG(node,storage_tec,level,year,time) =E=
* increase in the content of storage due to the activity of charging technologies - decrease due to discharging
        SUM((location,vintage,mode,tec,commodity,time2)$(map_tec_lifetime(node,tec,vintage,year) AND ) AND map_tec_storage_level(node,tec,storage_tec,level,year,time) ),
      (  output(location,tec,vintage,year,mode,node,commodity,level,time2,time) - input(location,tec,vintage,year,mode,node,commodity,level,time2,time) ) * ACT(location,tec,vintage,year,mode,time) )

but if we don't want to be so strict, and keep a level of freedom, I think the current implementation is ok.

in STORAGE_BALANCE Eq the if condition should not be with storage_loss otherwise the equation does not work then no losses is defined, probably map_tec_storage_level works well

Finally bound_storage_up and lo have the should have the same indexes as the STORAGE variable.
So we should decide whether keeping the commodity index in STORAGE or removing it from the boundary parameters

if we agree on some of these points I can also contribute to the code

Thanks a lot @gidden for the feedback and useful suggestions. I will move the documentation to the GAMS files as soon as we are happy with the final equations.
As for STORAGE_EQUALITY, based on the feedback from you and others, I replaced this with an optional parameter called relation_storage that can relate the content of storage between two time steps (and even between two different periods). This is now represented in equation STORAGE_REL as an inaquality to offer more flexiblity (vs. equality that forced the same content). I will continue improving the PR if you have further comments.

Thanks a lot @adrivinca for feedback and suggestions. I try to address them one by one:

• related to period_parameter_assignment and time_seq, I'm quite open if we decide to go for a set for adding the time order. Now, after removing STORAGE_EQUALITY we don't need to specify or map the first and last time steps anymore.
• related to list(set(seasons_dict.keys())), the order doesn't affect the results, as the duration will be specified for each time step directly in the loop.
• related to STORAGE_BALANCE, it is now mapped based on map_tec_storage_level.
• commodity was removed from the bounds and storage_loss.
  Please have a look again and suggest improvements if needed.

Hi all - just getting back and catching up. @behnam2015, does this need another review now?

@gidden, nothing from my side, @adrivinca may want to suggest some additions.

Hi folks, it looks like we have some conflicts in the GAMS code. I would suggest @behnam2015 @adrivinca and @danielhuppmann decide whether this is worth trying to get into the next release or should be a first PR into master after the release is finalized. I'm happy either way if all agree.

@behnam-zakeri I had to make the modifications in [iiasa/ixmp#315] ... Can you (1) please check that the code there, in combination with this branch, allows test_legacy_version to pass (it does for me), and (2) if so, leave an approving review there?

Thanks @khaeru for resolving this. Yes, both tests (storage and legacy) pass after these changes.

Unfortunately a test from message_ix/tests/test_tutorials.py::test_tutorial[austria_multiple_policies-answers] from the MacOS image is still failing:

  File "/Users/travis/miniconda/envs/testing/lib/python3.8/site-packages/nbconvert/preprocessors/execute.py", line 448, in preprocess_cell

    raise CellExecutionError.from_cell_and_msg(cell, out)

nbconvert.preprocessors.execute.CellExecutionError: An error occurred while executing the following cell:

------------------

new_scen = 'interest_rate_.1'

with make_scenario(mp, country, name, base_scen, new_scen) as scenario:

    scenario.add_par("interestrate", horizon, value=.1, unit='%')

Unfortunately a test from message_ix/tests/test_tutorials.py::test_tutorial[austria_multiple_policies-answers] from the MacOS image is still failing

@francescolovat good catch—see further down at line 1827:

com.zaxxer.hikari.pool.HikariPool.PoolInitializationException: com.zaxxer.hikari.pool.HikariPool$PoolInitializationException: Failed to initialize pool: Database lock acquisition failure: lockFile: org.hsqldb.persist.LockFile@5035d5ba[file =/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pytest-of-travis/pytest-0/localdb/default.lck, exists=true, locked=false, valid=false, ] method: checkHeartbeat read: 2020-04-30 15:54:05 heartbeat - read: -9763 ms.
com.zaxxer.hikari.pool.HikariPool.PoolInitializationException: com.zaxxer.hikari.pool.HikariPool$PoolInitializationException: Failed to initialize pool: Database lock acquisition failure: lockFile: org.hsqldb.persist.LockFile@5035d5ba[file =/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pytest-of-travis/pytest-0/localdb/default.lck, exists=true, locked=false, valid=false, ] method: checkHeartbeat read: 2020-04-30 15:54:05 heartbeat - read: -9763 ms.

This is a failure in the Java database layer that happens randomly once in a while; it is spurious (doesn't signal a code error). I will restart this job.

Ok @khaeru thanks! 🙂

Codecov Report

Merging #190 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   88.95%   88.95%           
=======================================
  Files          30       30           
  Lines        2697     2697           
=======================================
  Hits         2399     2399           
  Misses        298      298           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34ac6bf...34ac6bf. Read the comment docs.

Thanks @francescolovat for organizing the documentation very nicely and @khaeru for improving the model initialization workflow. I have no further additions or edits related to the formulation.